fix: persist new accessToken in SeedlessOnboarding Vault after tokens refresh#7800
fix: persist new accessToken in SeedlessOnboarding Vault after tokens refresh#7800
accessToken in SeedlessOnboarding Vault after tokens refresh#7800Conversation
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Show resolved
Hide resolved
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
…t Encryptor interface
…he wallet is unlocked
|
@metamaskbot publish-preview |
accessToken in SeedlessOnboarding Vault after tokens refresh
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Show resolved
Hide resolved
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Outdated
Show resolved
Hide resolved
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Outdated
Show resolved
Hide resolved
|
@metamaskbot publish-preview |
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Outdated
Show resolved
Hide resolved
|
@metamaskbot publish-preview |
1 similar comment
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Show resolved
Hide resolved
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Show resolved
Hide resolved
chaitanyapotti
left a comment
There was a problem hiding this comment.
Do we also refresh on server 401?
No, we don't refresh on 401 responses from Anyway, we validate the tokens (expiry) before we make requests (in |
| // if the password is provided (not undefined), encrypt the vault with the password | ||
| // We gonna prioritize the password encryption here, in case of the operation is `Change Password`. | ||
| // We don't wanna re-use the old encryption key from the state. | ||
| if (password !== undefined) { |
There was a problem hiding this comment.
best practice is to use typeof password !== "undefined" when checking for existence of a variable especially globals.
This is fine for this use case.
| // so skip the check if the vault is locked | ||
| let isAccessTokenExpired = false; | ||
| if (this.#isUnlocked) { | ||
| isAccessTokenExpired = this.checkAccessTokenExpired(); |
There was a problem hiding this comment.
Ideally, we must check expiry with server as part of oauth2 spec
since, client time can be incorrect or session might have been invalidated by server.
this particular implementation is non-standard and can be revisited at a later date
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
## Explanation <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> ## `@metamask/seedless-onboarding-controller` v8.0.0 This release adds vault encryption with a cached encryption key, a new `getAccessToken` public method with built-in token refresh, state log exclusions for sensitive token values, dependency bumps, and a fix for access token persistence after refresh. ### Breaking Changes - **The `encryptor` constructor param now requires an `encryptWithKey` method** ([#7800](#7800)) - This method is used to encrypt the vault with a cached encryption key while the wallet is unlocked. - Consumers must ensure the `encryptor` object passed to the constructor implements `encryptWithKey`. ### New Features - Added new public method `getAccessToken` ([#7800](#7800)) - Clients can use this method to retrieve the `accessToken` from the controller instead of directly accessing it from state. - This method includes a built-in refresh token mechanism when the `accessToken` is expired, preventing expired token usage in clients. ### Changes - Updated `StateMetadata`'s `includeInStateLogs` property to explicitly exclude all token values from state logs ([#7750](#7750)) ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> Fixes #7805 ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [x] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Version/changelog-only updates with no runtime code changes. > > **Overview** > Bumps the monorepo version to `808.0.0` and releases `@metamask/seedless-onboarding-controller` `8.0.0`. > > Updates the seedless onboarding controller changelog to include the `8.0.0` release entry and adjusts compare links accordingly. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit b67d0d6. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Explanation
This PR address
accessTokenisn't being persisted to vault after the tokens refresh (refreshAuthToken()).Issue:
Previously, clients could directly access state.accessToken from the controller state, which could potentially return an expired token. This could lead to failed API calls and poor user experience when the token had expired but wasn't refreshed.
Solution:
getAccessToken()that automatically refreshes expired tokens: Uses the #executeWithTokenRefresh wrapper to check token expiration and refresh if needed before returningaccessTokeninto Encrypted vault after token refresh.Persist
newAccessTokenin the encrypted vaultThe persist flow can be different based on the wallet (vault) status when
refreshAuthTokens()is called.encryptionKeyis available in the state).When wallet is unlocked, vault encryption key is temporarily cached in the controller state and the values inside the encrypted vault are also flooded into the controller state, for performance and to avoid the password dependency
We can use these cache values and create a new encrypted vault (with updated accessToken).
While the wallet is locked,
refreshAuthTokens()can be called under one condition, i.e, check for password sync (getAuthPubKeyRPC call) when user tries to unlock the wallet.In this case, we can't immediately update the encrypted vault as the wallet is locked. So, we will temporarily store the new
accessTokenin the controller state, then persist the stored value later in the vault unlock (only when user provides correct password) and update the vault.Based on the password sync status, the persist flow can be a little bit different.
2.1. Password is in sync
When user's current device password is in sync, we will update the vault when user unlocks via
submitPassword.Vault is decrypted using the submitted password, we will compare the access token value from decrypted vault (
accessToken1) and recently set new access token (accessToken2). If the accessToken is different and detected as new, we will update the encrypted vault value in the state (using password).Please note that this requires an additional encryption step to update the latestToken in encrypted vault.
2.2. Password is not in sync (
OutdatedPassword).When user's current device password is out of sync and user submits the latest
globalPassword, we do thepwEncKeyrecovery first so that we can unlock the current vault. Similar to the flow above (2.1), we decrypt the current vault and compare the two tokens (accessToken1andaccessToken2). If the accessToken is different and detected as new, we will update the controller state and encrypted vault.Unlike the flow (2.1.), this doesn't require additional encryption, as we can include the latest accessToken when we sync the latest global enc keys into the controller.
References
Fixes #7805, MetaMask/metamask-extension#39566
Checklist
Note
Medium Risk
Touches vault encryption/update paths and token refresh behavior, so bugs could lead to incorrect vault contents or token state during unlock/refresh flows, though changes are localized and well-covered by tests.
Overview
Fixes token persistence by updating the seedless onboarding vault when
refreshAuthTokens()obtains a newaccessToken, usingencryptWithKeywhen the wallet is already unlocked and reconciling state vs vault tokens duringsubmitPassword/submitGlobalPasswordvia a newcompareAndGetLatestTokenhelper.Adds a new public
getAccessToken()action/messenger handler that returns the current access token and triggers the existing refresh flow when tokens are expired, and introducesassertIsValidPasswordplus test refactors/new cases (including a sharedcreateMockJWTToken) to cover these behaviors. Breaking: the injected vaultencryptormust now implementencryptWithKey.Written by Cursor Bugbot for commit 09a0ddf. This will update automatically on new commits. Configure here.